Skip to content

OTA-1531: [1/x] cvo: refactor option processing #1184

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

petr-muller
Copy link
Member

@petr-muller petr-muller commented Apr 29, 2025

Extract option validation to a new ValidateAndComplete() method, and run it separate from Run() through cobra PreRunE feature.

Refactor the AlwaysEnabledCapabilities processing to be consistent with how the other options are processed. The NewControllerContext is a method on Options struct, so it does not make sense to pass it some options through the struct, and some options through method parameters. I went further and separated the validation (performed with all other validation) from conversion, resulting in two smaller methods instead one larger one. The result is slightly less efficient but more readable.

The reason to do this refactor is that I need to clean up the NewControllerContext constructor. I will need to do some changes around, and removing the explicit pass of one option-originated argument (while all others are passed by being Options members) will make the resulting code less messy. The reasoning for the changes:

  1. I need NewControllerContext to consume AlwaysEnabledCapabilities from the struct
  2. To consume it from the struct, it must be validated beforehands
  3. If it is validated beforehands, we do not need to track unknown and valid capabilities separately and validation can be simpler
  4. It would be useful if Run could generally rely on running with "all options were validated" assumption

The reason for this refactor is that I need to clean up the NewControllerContext constructor and the Context structure so that they do not become messy after the changes needed for OTA-1531. Check out #1182 for the probe coarse implementation of the full thing.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Apr 29, 2025
@openshift-ci-robot
Copy link
Contributor

@petr-muller: This pull request explicitly references no jira issue.

In response to this:

Extract option validation to a new ValidateAndComplete() method, and run it separate from Run() through cobra PreRunEa

Refactor the code to be consistent with how the other options are processed. The NewControllerContext is a method on Options struct, so it does not make sense to pass it some options through the struct, and some options through method parameters. I went further and separated the validation (performed with all other validation) from conversion, resulting in two smaller methods instead one larger one. The result is slightly less efficient but more readable.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 29, 2025
@petr-muller petr-muller force-pushed the cvo-options-refactor branch from fb3e267 to 4f68e05 Compare April 29, 2025 17:14
@petr-muller
Copy link
Member Author

petr-muller commented Apr 29, 2025

/hold

Should wait until branching at least

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 29, 2025
@petr-muller
Copy link
Member Author

/retest-required

@petr-muller
Copy link
Member Author

/skip

@petr-muller
Copy link
Member Author

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 2, 2025
@petr-muller petr-muller changed the title NO-JIRA: Refactor CVO option processing OTA-1531: [1/x] cvo: refactor option processing May 6, 2025
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented May 6, 2025

@petr-muller: This pull request references OTA-1531 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set.

In response to this:

Extract option validation to a new ValidateAndComplete() method, and run it separate from Run() through cobra PreRunE feature.

Refactor the AlwaysEnabledCapabilities processing to be consistent with how the other options are processed. The NewControllerContext is a method on Options struct, so it does not make sense to pass it some options through the struct, and some options through method parameters. I went further and separated the validation (performed with all other validation) from conversion, resulting in two smaller methods instead one larger one. The result is slightly less efficient but more readable.

The reason to do this refactor is that I need to clean up the NewControllerContext constructor. I will need to do some changes around, and removing the explicit pass of one option-originated argument (while all others are passed by being Options members) will make the resulting code less messy. The reasoning for the changes:

  1. I need NewControllerContext to consume AlwaysEnabledCapabilities from the struct
  2. To consume it from the struct, it must be validated beforehands
  3. If it is validated beforehands, we do not need to track unknown and valid capabilities separately and validation can be simpler
  4. It would be useful if Run could generally rely on running with "all options were validated" assumption

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented May 6, 2025

@petr-muller: This pull request references OTA-1531 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set.

In response to this:

Extract option validation to a new ValidateAndComplete() method, and run it separate from Run() through cobra PreRunE feature.

Refactor the AlwaysEnabledCapabilities processing to be consistent with how the other options are processed. The NewControllerContext is a method on Options struct, so it does not make sense to pass it some options through the struct, and some options through method parameters. I went further and separated the validation (performed with all other validation) from conversion, resulting in two smaller methods instead one larger one. The result is slightly less efficient but more readable.

The reason to do this refactor is that I need to clean up the NewControllerContext constructor. I will need to do some changes around, and removing the explicit pass of one option-originated argument (while all others are passed by being Options members) will make the resulting code less messy. The reasoning for the changes:

  1. I need NewControllerContext to consume AlwaysEnabledCapabilities from the struct
  2. To consume it from the struct, it must be validated beforehands
  3. If it is validated beforehands, we do not need to track unknown and valid capabilities separately and validation can be simpler
  4. It would be useful if Run could generally rely on running with "all options were validated" assumption

The reason for this refactor is that I need to clean up the NewControllerContext constructor and the Context structure so that they do not become messy after the changes needed for OTA-1531. Check out #1182 for the probe coarse implementation of the full thing.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

1 similar comment
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented May 6, 2025

@petr-muller: This pull request references OTA-1531 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set.

In response to this:

Extract option validation to a new ValidateAndComplete() method, and run it separate from Run() through cobra PreRunE feature.

Refactor the AlwaysEnabledCapabilities processing to be consistent with how the other options are processed. The NewControllerContext is a method on Options struct, so it does not make sense to pass it some options through the struct, and some options through method parameters. I went further and separated the validation (performed with all other validation) from conversion, resulting in two smaller methods instead one larger one. The result is slightly less efficient but more readable.

The reason to do this refactor is that I need to clean up the NewControllerContext constructor. I will need to do some changes around, and removing the explicit pass of one option-originated argument (while all others are passed by being Options members) will make the resulting code less messy. The reasoning for the changes:

  1. I need NewControllerContext to consume AlwaysEnabledCapabilities from the struct
  2. To consume it from the struct, it must be validated beforehands
  3. If it is validated beforehands, we do not need to track unknown and valid capabilities separately and validation can be simpler
  4. It would be useful if Run could generally rely on running with "all options were validated" assumption

The reason for this refactor is that I need to clean up the NewControllerContext constructor and the Context structure so that they do not become messy after the changes needed for OTA-1531. Check out #1182 for the probe coarse implementation of the full thing.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

Extract option validation to a new `ValidateAndComplete()` method, and run it separate from `Run()` through cobra `PreRunE`
Refactor the code to be consistent with how the other options are processed. The `NewControllerContext` is a method on `Options` struct, so it does not make sense to pass it some options through the struct, and some options through method parameters. I went further and separated the validation (performed with all other validation) from conversion, resulting in two smaller methods instead one larger one. The result is slightly less efficient but more readable.
@petr-muller petr-muller force-pushed the cvo-options-refactor branch from 4f68e05 to 40257c9 Compare May 6, 2025 16:16
@@ -511,7 +517,7 @@ func (o *Options) NewControllerContext(cb *ClientBuilder, alwaysEnableCapabiliti
o.PromQLTarget,
o.InjectClusterIdIntoPromQL,
o.UpdateService,
alwaysEnableCapabilities,
stringsToCapabilities(o.AlwaysEnableCapabilities),
Copy link
Member Author

@petr-muller petr-muller May 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the main point of the change - all other o.Members are passed into NewControllerContext via Options, and the method relies on their earlier validation. Treating one Options member in a different way makes the interface messy.

Copy link
Member

@wking wking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 6, 2025
Copy link
Contributor

openshift-ci bot commented May 6, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: petr-muller, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link
Contributor

openshift-ci bot commented May 6, 2025

@petr-muller: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/okd-scos-e2e-aws-ovn 40257c9 link false /test okd-scos-e2e-aws-ovn

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@petr-muller
Copy link
Member Author

/label no-qe

@openshift-ci openshift-ci bot added the no-qe Allows PRs to merge without qe-approved label label May 6, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit c410434 into openshift:main May 6, 2025
14 of 15 checks passed
@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

Distgit: cluster-version-operator
This PR has been included in build cluster-version-operator-container-v4.20.0-202505062216.p0.gc410434.assembly.stream.el9.
All builds following this will include this PR.

@petr-muller petr-muller deleted the cvo-options-refactor branch May 7, 2025 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. no-qe Allows PRs to merge without qe-approved label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants